Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix hyperlane types #1358

Closed
wants to merge 6 commits into from
Closed

Fix hyperlane types #1358

wants to merge 6 commits into from

Conversation

jmcardon
Copy link
Member

@jmcardon jmcardon commented Mar 27, 2024 •
edited by rsoeldner
Loading

Fixes our hyperlane type definitions to include the correct type schemas, as well as exporting schemas for hyperlane definitions

That is, we go from:

pact> hyperlane-message-id 
native `hyperlane-message-id`
  
  Get the Message Id of a Hyperlane Message object.
  
  Type:
  x:object:* -> string

to

pact> hyperlane-message-id 
native `hyperlane-message-id`
  
  Get the Message Id of a Hyperlane Message object.
  
  Type:
  x:object:{hyperlane-token-msg} -> string

As well as exporting the two necessary schemas:

pact> hyperlane-token-msg
(defschema
hyperlane-token-msg
"Schema type for hyperlane messages"

[ version:integer
, nonce:integer
, originDomain:integer
, destinationDomain:integer
, recipient:string
, tokenMessage:object:(defschema
hyperlane-token-erc20
"Schema type for ERC20 TokenMessage"

[recipient:string, amount:integer]) ])
pact> hyperlane-token-erc20
(defschema
hyperlane-token-erc20
"Schema type for ERC20 TokenMessage"

[recipient:string, amount:integer])

PR checklist:

  • Test coverage for the proposed changes
  • PR description contains example output from repl interaction or a snippet from unit test output
  • Documentation has been updated if new natives or FV properties have been added. To generate new documentation, issue cabal run tests. If they pass locally, docs are generated.

Additionally, please justify why you should or should not do the following:

  • Confirm replay/back compat
  • Will need replay
  • Benchmark regressions
  • There are functionally no changes to any implementations
  • (For Kadena engineers) Run integration-tests against a Chainweb built with this version of Pact
  • integration tests will need to be amended

0xd34df00d
0xd34df00d approved these changes Apr 2, 2024
View reviewed changes
src/Pact/Native.hs Outdated Show resolved Hide resolved
@rsoeldner rsoeldner mentioned this pull request Apr 2, 2024
Merged
8 tasks
@rsoeldner
fix hyperlane gas tests (#1359)
ba23e50 
* fix hyperlane gas tests
emilypi
emilypi reviewed Apr 2, 2024
View reviewed changes
Copy link
Member

@emilypi emilypi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved pending commentary from the @'d parties

src/Pact/Native.hs Show resolved Hide resolved
src/Pact/Native.hs
, (FieldKey "originDomain", tTyInteger)
, (FieldKey "destinationDomain", tTyInteger)
, (FieldKey "recipient", tTyString)
, (FieldKey "tokenMessage", tTyObject (TyUser (snd tokenMessageERC20Schema)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm @chessai or @edmundnoble should this or should this not contain a chainId field?

src/Pact/Native.hs
"hyperlane-message-id"
hyperlaneMessageId'
(funType tTyString [("x", tTyObjectAny)])
(funType tTyString [("x", tTyObject ty)])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice trick.

emilypi
emilypi requested changes Apr 2, 2024
View reviewed changes
Copy link
Member

@emilypi emilypi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to amend these docs too.

src/Pact/Native.hs
hyperlaneMessageIdDef = defGasRNative
tokenMessageERC20Schema :: NativeDef
tokenMessageERC20Schema = defSchema "hyperlane-token-erc20"
"Schema type for ERC20 TokenMessage"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kind of TokenMessage are we referring to here? If it's hyperlane specific it needs to mention this in the docs.

src/Pact/Native.hs


hyperlaneMessageIdDef :: Type (Term Name) -> NativeDef
hyperlaneMessageIdDef ty = defGasRNative
"hyperlane-message-id"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment needs work.

src/Pact/Native.hs
hyperlaneDecodeTokenMessageDef :: NativeDef
hyperlaneDecodeTokenMessageDef =
hyperlaneDecodeTokenMessageDef :: Type (Term Name) -> NativeDef
hyperlaneDecodeTokenMessageDef schematy =
defGasRNative
"hyperlane-decode-token-message"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

@jmcardon jmcardon marked this pull request as draft April 8, 2024 17:36
@jmcardon
Copy link
Member Author

jmcardon commented Apr 8, 2024

Drafting until I receive some feedback on the hyperlane msg format

@rsoeldner
Copy link
Member

closed for #1362

@rsoeldner rsoeldner closed this Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilypi emilypi emilypi requested changes

@0xd34df00d 0xd34df00d 0xd34df00d approved these changes

@jwiegley jwiegley Awaiting requested review from jwiegley jwiegley is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jmcardon @rsoeldner @0xd34df00d @emilypi